[Firestore] Add async counterpart for the addDocument method
#13407
Conversation
|
Thanks @MojtabaHs! I'd rather see this added to @ncooke3 @wu-hui Do you know if it was an oversight that this is missing or some other reason? |
Asked by paulb777 [here](firebase#13407 (comment))
| continuation.resume(returning: document!) | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
It catches the possible thrown error by calling the original addDocument function (which seems only throwable by the underlying encoder). Since withCheckedThrowingContinuation takes a non-throwing closure, we can't just rethrow it.
Am I missing something here?
There was a problem hiding this comment.
Oh I see. It looks like the problem is with the existing API above that both throws and does a callback with an error parameter.
There was a problem hiding this comment.
Agree, I was about to refactor it but stopped because it is part of the public API and would be backward-incompatible.
By the way, it's not clear to me what to return when encountering encoding errors.
I can open another PR for refactoring the existing API and discuss it further separately. Let me know your opinion.
There was a problem hiding this comment.
About this PR, how about moving this function back to the CollectionReference+AsyncAwait and using the existing API there like:
func addDocument<T: Encodable>(from value: T,
encoder: Firestore.Encoder = Firestore.Encoder()) async throws
-> DocumentReference {
let encoded = try encoder.encode(value)
return try await self.addDocument(data: encoded)
}
So no need for withCheckedThrowingContinuation at all.
Why move this to the other file? Because otherwise, this extension would depend on another extension.
Let me know your opinion.
There was a problem hiding this comment.
See #9157 (comment) for a discussion about an edge case when the client is offline. This would result in the async method never returning.
There was a problem hiding this comment.
Using this method will depend on the existing async API. So, at the end of the day, we should either deprecate both of them or agree on the current existing logic.
MojtabaHs
changed the title
addDocument method
|
@MojtabaHs Thanks for the PR. Given the open questions we're seeing, we want to a deeper review of the Firestore APIs and make sure we're being consistent with how we handle |
|
@paulb777 Let me know if there is anything I can do to help this progress. |
|
Any updates on this? There are other opportunities to introduce async counterparts to modernize the API, but I’m concerned that this decision could affect those efforts as well. I believe it’s important to address this first. |
|
Hi @MojtabaHs, no updates as of yet. This is something actively being looked into. I'll update when the direction is more clear. |
|
It's been a while, how can I assist you in moving forward with this progress? |
Asked by paulb777 [here](firebase#13407 (comment))
MojtabaHs
force-pushed
the
Firestore-async-counterparts
branch
from
af4e2b8
to
77e8c60
Compare
|
Rebased on the current main branch. Any updates? It’s been a while, and I’m concerned about losing context. |
The async version of the
addDocumentfunction exists only forDictionaryinput, so I have added acodableversion as well using the original existing methods.addDocumentwhich takes an encodable.